Ci speedup #845
Ci speedup #845
Conversation
🐛 fix snapshot name conflict
⚡ speed up race cond
🚀 boost CI speed
| . venv/bin/activate | ||
| pytest --junitxml=test-reports/junit_intg.xml tests/integration/ | ||
| cd packages && ls -la && pip install * && cd .. && pip list | grep dash | ||
| TESTFILES=$(circleci tests glob "tests/integration/**/test_*.py" | circleci tests split --split-by=timings) |
There was a problem hiding this comment.
circleci way to split and select tests based on previous execution duration infos
| docker: | ||
| - image: percyio/agent | ||
| steps: | ||
| - run: percy finalize --all |
There was a problem hiding this comment.
this works together with a PERCY_PARALLEL_TOTAL: '-1' in test job, percy command will compare the number of containers by fetching ENV variable value from circleci.
| - run: echo $PYVERSION > ver.txt | ||
| - restore_cache: | ||
| key: v-{{ checksum "circlejob.txt" }}-{{ checksum "requires-ci.txt" }}-{{ checksum "requires-install.txt" }}-{{ checksum "requires-testing.txt" }} | ||
| key: dep-{{ checksum "ver.txt" }}-{{ checksum "requires-ci.txt" }}-{{ checksum "requires-install.txt" }}-{{ checksum "requires-testing.txt" }} |
There was a problem hiding this comment.
the cache key formula might need further tuning later
|
the whole config.yml can be improved using some new features in version 2.1, but out of the scope of this PR => speed up the CI time. |
|
we might need to change the documents as the current change have an impact on local circlecli runs. |
|
|
||
|
|
||
| DELAY_TIME = 1 | ||
| DELAY_TIME = 0.2 |
There was a problem hiding this comment.
heh that in itself should be a huge speedup. Should be just fine - if for some reason that's ever not a long enough delay to actually have the responses arrive at the front end in the desired order, it would be a (presumably rare) missed test, or testing a different order, but one that should still be valid. So I'm happy to make this change, but some day we should refactor this test to something a little smarter that actually triggers the responses based on evidence that the previous response has been received and acted upon, rather than just time.
| self.wait_for_text_to_equal('#output-post-payload', '{"output":"output-1.children","changedPropIds":["input.value"],"inputs":[{"id":"input","property":"value","value":"fire request hooks"}]}') | ||
| self.wait_for_text_to_equal('#output-post-response', '{"props":{"children":"fire request hooks"}}') | ||
| self.percy_snapshot(name='request-hooks') | ||
| self.percy_snapshot(name='request-hooks render') |
There was a problem hiding this comment.
oh interesting - was one of these snapshots blocking the other one previously? How did you discover this? Do we need some additional mechanism (like a list of previously-used names inside percy_snapshot - though I guess in this case that wouldn't help as one's the new framework and the other is still using the old system) to ensure we don't double up a name again?
There was a problem hiding this comment.
And I guess the next question: do these two tests actually both have distinct value, or should we delete one of them?
There was a problem hiding this comment.
I got a 400 error when the test job start to have parallelism > 1
There was a problem hiding this comment.
oh weird - so it allows multiple snapshots with the same name from the same process but not from different processes? very strange, though useful I guess as any test we do in our own code could only catch duplicates in the same process.
There was a problem hiding this comment.
there was another issue with the previous approach, as we spin up a browser per test vs per session, the percy_finalize was called after each test case.
anyway, there are some extra integration under the hood from percy client, it adds some extra protection for all major CI tools.
There was a problem hiding this comment.
Ah, got it - right, it's probably the extra finalizing that buried the duplicate, and now percy will be taking care of ensuring uniqueness.
alexcjohnson
left a comment
alexcjohnson
left a comment
There was a problem hiding this comment.
💃 Thanks for doing this, should make us all more productive!
this fixes #841
Start with a description of this PR. Then edit the list below to the items that make sense for your PR scope, and check off the boxes as you go!
Contributor Checklist